Improving terraform taggable resources list #375

lonegunmanb · 2023-05-25T11:55:26Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

As #374 described, this pr change the static taggable resources list to a calculated list by using terraform-aws-schema / terraform-azurerm-schema / terraform-google-schema.

This pr also implemented the following logics:

Once a taggable resource was deprecated in the later major version (e.g.: azurerm_data_lake_analytics_account was deprecated and deleted in AzureRM provider v3.0), is should still be taggable for backward compatibility.
Once a taggable resource's tags argument was deprecated and deleted (e.g.: azurerm_log_analytics_linked_service's tags has been removed by this pr), it should not be taggable.
A resource which contains tags but with different type (map(string) for aws and azurerm, set(string) for google) should not be taggable. (e.g.: azurerm_api_management_property)
Resources in structure.unsupportedTerraformBlocks should not be taggable.

I was unable to generate schemas for awsv0, awsv1, azurermv0, azurermv1, googlev0, googlev1, so the lowest major version for these three clouds is v2. For now it supports awsv2, awsv3, awsv4, awsv5, azurermv2, azurermv3, googlev2, googlev3, googlev4. Once a new major version of provider has been released, there won't be new corresponding schema tag, we need update the schema repo to generate new package version for schema repo, and upgrade yor's code to adapt with the new schema package.

This pr contains some new unit tests to verify these logics. @nimrodkor Would you please give it a review? Thanks!

lonegunmanb · 2023-05-25T13:13:45Z

It looks like bug in the latest go-git version (v5.7.0), I'll try to downgrade go-git version for schema repo and try again tomorrow.

lonegunmanb · 2023-05-26T03:03:46Z

I was unable to downgrade the go-git version... It seems like the only way is solve this go-git issue.

stale · 2023-06-28T06:09:23Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

lonegunmanb · 2023-06-30T03:03:59Z

please keep it open

lonegunmanb · 2023-07-24T09:46:56Z

I have upgraded go-git version to 5.8.0 in this pr to get tests passed. @nimrodkor @gruebel @ChanochShayner I think this pr is ready for your review finally.

gruebel

great work, especially making the git dependency compatible with us again 💪

gruebel · 2023-07-24T11:47:43Z

src/terraform/structure/tf_taggable.go

+var TfTaggableResourceTypes []string
+
+func init() {
+	linq.From(previousTaggableTypes(awsv4.Resources, isTaggableType, awsv2.Resources, awsv3.Resources, awsv4.Resources)).


Suggested change

linq.From(previousTaggableTypes(awsv4.Resources, isTaggableType, awsv2.Resources, awsv3.Resources, awsv4.Resources)).

linq.From(previousTaggableTypes(awsv4.Resources, isTaggableType, awsv2.Resources, awsv3.Resources)).

it looks like you have aws4 twice in the line

gruebel · 2023-07-24T11:52:42Z

src/terraform/structure/tf_taggable.go

+
+var TfTaggableResourceTypes []string
+
+func init() {


do you know how long the initialization takes? this will be recreated each time the CLI is used or am I missing something?

I'll do a benchmark so you can make the call based on that. I was thinking about converting the json marshal operation into a lazy load, but it wasn't my priority before. If you're not satisfied with the benchmark result, then I'll make the load to lazy load.

Hi @gruebel I just recalled that we cannot boost the init by lazy load since we need to filter out the resources that contains tags. Here's the benchmark on my laptop and if you're not satisfied with it, maybe we can boost the load by code generate?:

goos: windows goarch: amd64 pkg: github.com/bridgecrewio/yor/src/terraform/structure cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz BenchmarkInit BenchmarkInit-8 246 5656524 ns/op BenchmarkInit-8 270 4959608 ns/op BenchmarkInit-8 211 5061141 ns/op BenchmarkInit-8 249 4825400 ns/op BenchmarkInit-8 310 3281290 ns/op BenchmarkInit-8 396 3308266 ns/op BenchmarkInit-8 387 3507547 ns/op BenchmarkInit-8 358 3308801 ns/op BenchmarkInit-8 357 3374912 ns/op BenchmarkInit-8 360 3287701 ns/op BenchmarkInit-8 396 3366633 ns/op BenchmarkInit-8 386 3320744 ns/op BenchmarkInit-8 391 5482833 ns/op BenchmarkInit-8 213 4862594 ns/op BenchmarkInit-8 285 3868470 ns/op BenchmarkInit-8 303 3673442 ns/op BenchmarkInit-8 354 3305469 ns/op BenchmarkInit-8 338 3350286 ns/op BenchmarkInit-8 387 3335693 ns/op BenchmarkInit-8 358 3304018 ns/op BenchmarkInit-8 361 3436961 ns/op BenchmarkInit-8 350 3917158 ns/op BenchmarkInit-8 332 3217946 ns/op BenchmarkInit-8 388 3363009 ns/op BenchmarkInit-8 348 3279345 ns/op BenchmarkInit-8 367 3206369 ns/op BenchmarkInit-8 391 3257789 ns/op BenchmarkInit-8 368 3270909 ns/op BenchmarkInit-8 352 3235348 ns/op BenchmarkInit-8 386 3352852 ns/op BenchmarkInit-8 318 3525487 ns/op BenchmarkInit-8 362 3526662 ns/op BenchmarkInit-8 333 3532842 ns/op BenchmarkInit-8 376 3321541 ns/op BenchmarkInit-8 356 3308681 ns/op BenchmarkInit-8 360 3678722 ns/op BenchmarkInit-8 367 3250190 ns/op BenchmarkInit-8 364 3266191 ns/op BenchmarkInit-8 363 3474205 ns/op BenchmarkInit-8 340 3352039 ns/op BenchmarkInit-8 338 3202551 ns/op BenchmarkInit-8 340 3348379 ns/op BenchmarkInit-8 320 3288201 ns/op BenchmarkInit-8 374 3283416 ns/op BenchmarkInit-8 338 3218595 ns/op BenchmarkInit-8 376 3346400 ns/op BenchmarkInit-8 368 3320164 ns/op BenchmarkInit-8 316 3336018 ns/op BenchmarkInit-8 326 3993416 ns/op BenchmarkInit-8 302 3397222 ns/op BenchmarkInit-8 342 3320784 ns/op BenchmarkInit-8 344 3186624 ns/op BenchmarkInit-8 327 3182577 ns/op BenchmarkInit-8 373 3342972 ns/op BenchmarkInit-8 358 3381648 ns/op BenchmarkInit-8 357 3348296 ns/op BenchmarkInit-8 355 3211068 ns/op BenchmarkInit-8 331 3260557 ns/op BenchmarkInit-8 348 3390506 ns/op BenchmarkInit-8 358 3988988 ns/op BenchmarkInit-8 376 3296542 ns/op BenchmarkInit-8 367 3203626 ns/op BenchmarkInit-8 330 3174616 ns/op BenchmarkInit-8 378 3210405 ns/op BenchmarkInit-8 346 3271160 ns/op BenchmarkInit-8 358 3351525 ns/op BenchmarkInit-8 339 3178830 ns/op BenchmarkInit-8 285 3530353 ns/op BenchmarkInit-8 330 3334848 ns/op BenchmarkInit-8 386 3372618 ns/op BenchmarkInit-8 333 3274653 ns/op BenchmarkInit-8 355 3671500 ns/op BenchmarkInit-8 322 3359247 ns/op BenchmarkInit-8 366 3307886 ns/op BenchmarkInit-8 343 3992340 ns/op BenchmarkInit-8 358 3436902 ns/op BenchmarkInit-8 384 3342238 ns/op BenchmarkInit-8 342 3323672 ns/op BenchmarkInit-8 357 3265490 ns/op BenchmarkInit-8 372 3465086 ns/op BenchmarkInit-8 380 3311641 ns/op BenchmarkInit-8 333 3364876 ns/op BenchmarkInit-8 369 3275553 ns/op BenchmarkInit-8 393 3413220 ns/op BenchmarkInit-8 310 3503894 ns/op BenchmarkInit-8 331 3305068 ns/op BenchmarkInit-8 393 3400776 ns/op BenchmarkInit-8 357 3284698 ns/op BenchmarkInit-8 364 4481733 ns/op BenchmarkInit-8 307 3947393 ns/op BenchmarkInit-8 333 3271755 ns/op BenchmarkInit-8 241 6217576 ns/op BenchmarkInit-8 361 3335206 ns/op BenchmarkInit-8 343 3278905 ns/op BenchmarkInit-8 355 3612213 ns/op BenchmarkInit-8 348 3298262 ns/op BenchmarkInit-8 390 3479846 ns/op BenchmarkInit-8 406 3255017 ns/op BenchmarkInit-8 355 3320015 ns/op BenchmarkInit-8 349 3255885 ns/op PASS Process finished with the exit code 0

lonegunmanb · 2023-08-17T05:03:20Z

@gruebel A kindly ping, is there anything I can do to get this pr be merged?

…fy an unit test.

lonegunmanb · 2023-09-11T03:13:05Z

Hi @gruebel a kindly ping, I've rebased this pr with the latest main branch and upgraded the module's version. I saw security step failed once but I cannot see the detail. Would you please try this pr again and guide me how to fix the security scan issue if it failed again? Thanks!

lonegunmanb · 2023-09-20T06:14:47Z

Hello @nimrodkor @gruebel a kindly ping, I think this pr is ready for your review, and the update it brought is very important for some community users. Could we merge it? Thanks!

stale · 2023-10-20T06:50:11Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

nimrodkor · 2023-11-01T20:39:40Z

@rotemavni @gruebel @omryMen PTAL 🙏

stale · 2023-12-02T05:26:34Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

nimrodkor requested review from rotemavni and omryMen May 28, 2023 07:00

stale bot added the wontfix This will not be worked on label Jun 28, 2023

stale bot removed the wontfix This will not be worked on label Jun 30, 2023

lonegunmanb force-pushed the tf-taggable branch from ad2f508 to 245e150 Compare July 24, 2023 09:18

lonegunmanb had a problem deploying to scan-security July 24, 2023 09:18 — with GitHub Actions Failure

lonegunmanb had a problem deploying to scan-security July 24, 2023 09:34 — with GitHub Actions Failure

lonegunmanb temporarily deployed to scan-security July 24, 2023 09:42 — with GitHub Actions Inactive

gruebel reviewed Jul 24, 2023

View reviewed changes

lonegunmanb had a problem deploying to scan-security July 25, 2023 00:06 — with GitHub Actions Failure

lonegunmanb had a problem deploying to scan-security July 25, 2023 00:13 — with GitHub Actions Failure

lonegunmanb had a problem deploying to scan-security July 28, 2023 10:11 — with GitHub Actions Failure

lonegunmanb requested a review from gruebel July 28, 2023 10:12

lonegunmanb mentioned this pull request Aug 9, 2023

Unable to fetch git blame info on specific files #155

Closed

lonegunmanb had a problem deploying to scan-security September 11, 2023 02:03 — with GitHub Actions Failure

lonegunmanb and others added 9 commits September 11, 2023 10:04

commit for now

8dcaedc

finish the feature

ad47933

upgrade dependency

50dc9d1

fix lint issue

178c8e8

kick off ci again

e5d6912

bump go-git version

00b1277

fix incorrect aws schema load, add benchmark for schema load

2411a7a

restart ci

a89a31b

rebase with latest main branch, upgrade dependencies versions, simpli…

81a5058

…fy an unit test.

lonegunmanb force-pushed the tf-taggable branch from 29cef68 to 81a5058 Compare September 11, 2023 02:05

lonegunmanb temporarily deployed to scan-security September 11, 2023 02:05 — with GitHub Actions Inactive

stale bot added the wontfix This will not be worked on label Oct 20, 2023

stale bot removed the wontfix This will not be worked on label Nov 1, 2023

stale bot added the wontfix This will not be worked on label Dec 2, 2023

stale bot closed this Jan 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving terraform taggable resources list #375

Improving terraform taggable resources list #375

lonegunmanb commented May 25, 2023 •

edited

Loading

lonegunmanb commented May 25, 2023

lonegunmanb commented May 26, 2023

stale bot commented Jun 28, 2023

lonegunmanb commented Jun 30, 2023

lonegunmanb commented Jul 24, 2023 •

edited

Loading

gruebel left a comment

gruebel Jul 24, 2023

lonegunmanb Jul 24, 2023

gruebel Jul 24, 2023

lonegunmanb Jul 24, 2023

lonegunmanb Jul 25, 2023 •

edited

Loading

lonegunmanb commented Aug 17, 2023

lonegunmanb commented Sep 11, 2023

lonegunmanb commented Sep 20, 2023

stale bot commented Oct 20, 2023

nimrodkor commented Nov 1, 2023

stale bot commented Dec 2, 2023

	linq.From(previousTaggableTypes(awsv4.Resources, isTaggableType, awsv2.Resources, awsv3.Resources, awsv4.Resources)).
	linq.From(previousTaggableTypes(awsv4.Resources, isTaggableType, awsv2.Resources, awsv3.Resources)).

Improving terraform taggable resources list #375

Improving terraform taggable resources list #375

Conversation

lonegunmanb commented May 25, 2023 • edited Loading

lonegunmanb commented May 25, 2023

lonegunmanb commented May 26, 2023

stale bot commented Jun 28, 2023

lonegunmanb commented Jun 30, 2023

lonegunmanb commented Jul 24, 2023 • edited Loading

gruebel left a comment

Choose a reason for hiding this comment

gruebel Jul 24, 2023

Choose a reason for hiding this comment

lonegunmanb Jul 24, 2023

Choose a reason for hiding this comment

gruebel Jul 24, 2023

Choose a reason for hiding this comment

lonegunmanb Jul 24, 2023

Choose a reason for hiding this comment

lonegunmanb Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

lonegunmanb commented Aug 17, 2023

lonegunmanb commented Sep 11, 2023

lonegunmanb commented Sep 20, 2023

stale bot commented Oct 20, 2023

nimrodkor commented Nov 1, 2023

stale bot commented Dec 2, 2023

lonegunmanb commented May 25, 2023 •

edited

Loading

lonegunmanb commented Jul 24, 2023 •

edited

Loading

lonegunmanb Jul 25, 2023 •

edited

Loading